Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maximal angles between convex cones #37854

Merged
merged 71 commits into from
May 12, 2024

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Apr 23, 2024

My colleagues are writing about this stuff again. Let's migrate #29169 from the old Trac branch.

Relative to develop:

  • Add a new reference
  • Add a new module src.sage.geometry.cone_critical_angles for the implementation
  • Add a max_angle() method to the class in src.sage.geometry.cone for the user interface

Relative to the old trac branch:

TODO:

  • Revert commit 95e5763 after checking the docs

Copy link

github-actions bot commented Apr 23, 2024

Documentation preview for this PR (built with commit 1b8f08f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@orlitzky orlitzky force-pushed the maximal-angles-between-cones branch 2 times, most recently from 6e0433d to 66eba0e Compare April 24, 2024 16:11
@orlitzky orlitzky force-pushed the maximal-angles-between-cones branch 2 times, most recently from c0a73cf to ab954c6 Compare April 25, 2024 00:46
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully these changes are all speed improvements. I didn't check carefully that all statements are equivalent, but hopefully the intent is clear. Let me know if you have questions.

src/sage/geometry/cone.py Outdated Show resolved Hide resolved
src/sage/geometry/cone.py Outdated Show resolved Hide resolved
src/sage/geometry/cone.py Outdated Show resolved Hide resolved
src/sage/geometry/cone.py Outdated Show resolved Hide resolved
src/sage/geometry/cone_critical_angles.py Outdated Show resolved Hide resolved
src/sage/geometry/cone_critical_angles.py Outdated Show resolved Hide resolved
src/sage/geometry/cone_critical_angles.py Outdated Show resolved Hide resolved
src/sage/geometry/cone_critical_angles.py Outdated Show resolved Hide resolved
src/sage/geometry/cone_critical_angles.py Outdated Show resolved Hide resolved
src/sage/geometry/cone_critical_angles.py Outdated Show resolved Hide resolved
orlitzky added 15 commits May 4, 2024 09:47
The phrase "this cone cannot be..." can be misinterpreted as making a
value judgment about the given cone, rather than a blanket statement
about the argument (self) to these two functions. This commit changes
it to "cone should not be..." to indicate that the user should not
pass in either a trivial cone or the ambient space.
…e().

The error message output when _random_admissible_cone() receives an
invalid ambient dimension was a full sentence. This commit pares it
down to just the important part; amely, that there are no nontrivial
cones in the ambient dimension.
The line "G_index_sets = list(gevp_licis(G))" appears twice in EXAMPLES,
but the variable "G_index_sets" is not used thereafter. It was most
likely copy/pasted into the example by mistake. This commit removes it.
…30605.

Thanks to some recent improvements in cone containment testing, we no
longer have to reimplement it for vectors with irrational entries
ourselves.
The sage library is now doctested with a random random seed (imagine),
making most calls to set_random_seed() superfluous. We remove a few
from the cone critical angle code here.
The solve_gevp_zero() and solve_gevp_nonzero() functions for cone
critical angles are "internal" and can return generators. Likewise for
the testing function _solve_gevp_naive(). This should be a tiny bit
more efficient.
…metic.

Passing exact=False to either the max_angle() or critical_angles()
method of cones is a bit "dangerous" because we can miss eigenspaces
of dimension >= 2 due to numerical issues. Add a warning to the
docstring about it.
…s.py.

Two tests in this module imported the gevp_licis() function but then
did not use it. No longer.
We already have a matroid method that does more or less what the new
gevp_licis() function does in cone_critical_angles.py. It's faster to
use the matroid implementation and then tweak its output slightly, so
let's do that instead.
For reasons I've long forgotten, the cone max/critical angles
algorithms were using QQbar for "rationals with roots." Switching it
to AA causes no problems now, and is clearly preferable (the only
eigenvalues we compute are of positive-definite matrices).
@orlitzky orlitzky force-pushed the maximal-angles-between-cones branch from 26793ea to 1e742e9 Compare May 4, 2024 16:31
Using the identity M[I,J].transpose() == M.transpose()[J,I], we can
avoid transposing M itself and instead transpose the smaller M[I,J]
when M[I,J] is already known.
…ning

The existing warning about this module being internal is easy to
miss. This one should appear in bright red at the top of the module
documentation.
…ngles

This is an internal module, but it's fully documented and nice to have
access to the implementation behind the max_angle() function. Despite
being documented, the functions in cone_critical_angles.py are subject
to change at any time. This itself is loudly documented.
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thank you. Then let it be so.

@orlitzky
Copy link
Contributor Author

orlitzky commented May 7, 2024

Thanks for the review!

vbraun pushed a commit to vbraun/sage that referenced this pull request May 9, 2024
    
My colleagues are writing about this stuff again. Let's migrate
sagemath#29169 from the old Trac branch.

Relative to _develop_:
* Add a new reference
* Add a new module `src.sage.geometry.cone_critical_angles` for the
implementation
* Add a `max_angle()` method to the class in `src.sage.geometry.cone`
for the user interface

Relative to the old trac branch:

* Update the algorithm to take
https://www.heldermann.de/JCA/JCA31/JCA311/jca31015.htm into
consideration
* Add that paper as a reference
* Drop the more-general critical angles computation. It isn't nearly as
nice as the maximal angle method.

TODO:

* ~~Revert commit 95e5763 after checking the docs~~
    
URL: sagemath#37854
Reported by: Michael Orlitzky
Reviewer(s): Kwankyu Lee, Matthias Köppe, Michael Orlitzky, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
    
My colleagues are writing about this stuff again. Let's migrate
sagemath#29169 from the old Trac branch.

Relative to _develop_:
* Add a new reference
* Add a new module `src.sage.geometry.cone_critical_angles` for the
implementation
* Add a `max_angle()` method to the class in `src.sage.geometry.cone`
for the user interface

Relative to the old trac branch:

* Update the algorithm to take
https://www.heldermann.de/JCA/JCA31/JCA311/jca31015.htm into
consideration
* Add that paper as a reference
* Drop the more-general critical angles computation. It isn't nearly as
nice as the maximal angle method.

TODO:

* ~~Revert commit 95e5763 after checking the docs~~
    
URL: sagemath#37854
Reported by: Michael Orlitzky
Reviewer(s): Kwankyu Lee, Matthias Köppe, Michael Orlitzky, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
    
My colleagues are writing about this stuff again. Let's migrate
sagemath#29169 from the old Trac branch.

Relative to _develop_:
* Add a new reference
* Add a new module `src.sage.geometry.cone_critical_angles` for the
implementation
* Add a `max_angle()` method to the class in `src.sage.geometry.cone`
for the user interface

Relative to the old trac branch:

* Update the algorithm to take
https://www.heldermann.de/JCA/JCA31/JCA311/jca31015.htm into
consideration
* Add that paper as a reference
* Drop the more-general critical angles computation. It isn't nearly as
nice as the maximal angle method.

TODO:

* ~~Revert commit 95e5763 after checking the docs~~
    
URL: sagemath#37854
Reported by: Michael Orlitzky
Reviewer(s): Kwankyu Lee, Matthias Köppe, Michael Orlitzky, Travis Scrimshaw
@vbraun vbraun merged commit 08be24c into sagemath:develop May 12, 2024
17 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants